Fix a few bugs in opentelemetry-instrumentation-google-genai instrumentation package
#3905
Conversation
DylanRussell
changed the title
DylanRussell
changed the title
opentelemetry-instrumentation-google-genai instrumentation package
| if hasattr(value, "model_dump"): | ||
| return value.model_dump() | ||
| try: | ||
| return value.model_dump() | ||
| except TypeError: | ||
| return {"ModelName": str(value)} | ||
|
|
There was a problem hiding this comment.
Can we just make this function typed since we know we're receiving GenerateContentConfigOrDict? I think it's "provable" that we don't need this hasattr check and that the call is correct
One issue is I don't think this file is opted in to pyright 🙁 but doesn't change the implementation
| if config.response_mime_type: | ||
| if config.response_mime_type == "text/plain": | ||
| response_mime_type = config.get("response_mime_type") | ||
| if response_mime_type and is_experimental_mode: |
There was a problem hiding this comment.
Will this make the GEN_AI_OUTPUT_TYPE go away for people not using experimental semconv?
Looking at the test seems like none of the "old" semconv changes
| config.response_mime_type | ||
| response_mime_type | ||
| ) | ||
| for key in list(attributes.keys()): |
There was a problem hiding this comment.
| for key in list(attributes.keys()): | |
| for key in attributes: |
...trumentation-google-genai/src/opentelemetry/instrumentation/google_genai/generate_content.py
Show resolved
Hide resolved
| self, | ||
| request_attributes: dict[str, Any], | ||
| final_attributes: dict[str, Any], | ||
| is_experimental_mode: bool, |
There was a problem hiding this comment.
I think you can just use self.sem_conv_opt_in_mode
| span.set_attribute( | ||
| gen_ai_attributes.GEN_AI_SYSTEM, helper._genai_system | ||
| ) |
There was a problem hiding this comment.
I'm surprised helper doesn't set this itself, that would be simpler imo. Feel free to disregard not too familiar with this code
| def configure_exception(self, e, **kwargs): | ||
| self._create_and_install_mocks(e) | ||
|
|
||
| def _create_and_install_mocks_with(self): |
There was a problem hiding this comment.
doesn't look like this function is used
| if not e: | ||
| mock.side_effect = _default_impl | ||
| else: | ||
| mock.side_effect = e |
There was a problem hiding this comment.
nit feel free to ignore
| if not e: | |
| mock.side_effect = _default_impl | |
| else: | |
| mock.side_effect = e | |
| mock.side_effect = e or _default_impl |
| try: | ||
| self.generate_content( | ||
| model="gemini-2.0-flash", contents="Does this work?" | ||
| ) | ||
| except ValueError: |
There was a problem hiding this comment.
instrumentation-genai/opentelemetry-instrumentation-google-genai/CHANGELOG.md
Show resolved
Hide resolved
6 participants
Description
pydantic.BaseModelclass was passed as the response schema toGenerateContentConfig, the code expected a class instance for some reason (see BaseModel.model_dump() called on class instead of instance in opentelemetry-instrumentation-google-genai #3596).error.typesem conv attribute when there's an exception raised._maybe_log_completion_details(function call into thefinallyblock for the 4 instrumentation variants (sync/async streaming/non). This ensures we always write the event and call the completion hook etc. even when there's an exception thrown.gen_ai.client.inference.operation.detailslog event is written and completion upload hook are called whenOTEL_SEMCONV_STABILITY_OPT_IN=gen_ai_latest_experimental.Fixes #3596
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Unit tests
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.